Fix nominator_bond_less deletes bottom nomination bug #748
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4meta5
added
A3-inprogress
4meta5
added
B7-runtimenoteworthy
4meta5
commented
Aug 29, 2021
pallets/parachain-staking/src/lib.rs
Outdated
Comment on lines
393
to
395
| } else { | ||
| // reset self.bottom_nominators | ||
| self.bottom_nominators.push(top_bottom); |
There was a problem hiding this comment.
Without this line, the code above would pop the top bottom nomination every time this was called. This pop led to inconsistent Collator2 state, preventing proper exit by unreserving reserved balance.
We can find the nominators that were incorrectly removed by this bug by searching the CollatorState2.nominators: Vec<AccountId> for an account that is not in the top or bottom nominator sets. This is done in the migration code. If the nominator did not exit, the nomination might be in their NominatorState2.
There was a problem hiding this comment.
This line is what patches the bug by the way. We forgot to push back after pop and it led to inadvertently removing nominations from CollatorState.
…gration unit test
…ed non nominators
…ve no nominations after it runs
notlesh
reviewed
Aug 29, 2021
…ng storage write functionality to outside of crate
|
Process to test it: Get the fork of Moonriver
File available there: https://drive.google.com/file/d/1KmMsO4JvcvSDgl7dqzaY8Ruwe4woxNsV/view?usp=sharing Start the nodeVerify the data contains invalid stateRequires branch At the Performing runtime upgrade (with data migration)Using this branch, and changing the runtime/moonriver/lib.rs spec_version to Using proposal to Refreshing polkadotJs page to see the Verifying the data migrationVerify the bug is not happening anymore
Run the script using from We can see the events, and there is no report (inside Double verification
Run the script using from The error is still there, meaning the bug was in runtime-400 and the new runtime of this branch is not triggering it |
|
https://github.com/PureStake/fork-off-substrate can be used to produce a new genesis file if needed (I'd use Alan's file if at all possible though). It includes all of the tweaks mentioned except for modifying the collator to be alice. |
|
Was |
|
I didn't think of checking the total. I can add it to the script |
|
@4meta5 according to this data (#425111), the total seems broken too: |
* init naive not tested * fix compilation errors, still needs tests * test
|
@crystalin both totals should be reset correctly after #751 was merged into this branch, but I was talking about storage value |
girazoki
reviewed
Aug 30, 2021
librelois
previously requested changes
Aug 30, 2021
…in memory instead
girazoki
approved these changes
Aug 30, 2021
girazoki
reviewed
Aug 30, 2021
Co-authored-by: girazoki <gorka.irazoki@gmail.com>
notlesh
approved these changes
Aug 30, 2021
4meta5
removed
the
A3-inprogress
crystalin
deleted the
patch-staking-bond-less-deletes-bottom-nominator-bug
branch
crystalin
pushed a commit
that referenced
this pull request
Aug 30, 2021
* in progress * update top nominators list upon changes and do not remove any bottom nominations * test fix * init migration to start to fix inconsistent state * more progress on migration * improve nomination increase decrease events * fmt * review and comment all uses of pop * add two different errors for each source of inconsistency and init migration unit test * add unreserve stakers extrinsic gated by root to unreserve the reserved non nominators * in middle of testing migration and found bug wherein nominator can have no nominations after it runs * fix migration, still needs weights * rename root unreserve function and finish migration test * last few changes to public visibility to keep the test without exposing storage write functionality to outside of crate * fix comment * Fix max nominators per collator upgrade bug (#751) * init naive not tested * fix compilation errors, still needs tests * test * fix total staked in hotfix_unreserve_nomination as well * accept review comments * fix weight returned in on runtime upgrade * remove accounts due unreserved balance storage item and use btreemap in memory instead * try fix * no filtering condition on first migration loop * merge migration functions into one function * conservative weight estimate * Update pallets/parachain-staking/src/lib.rs Co-authored-by: girazoki <gorka.irazoki@gmail.com> Co-authored-by: girazoki <gorka.irazoki@gmail.com>
This was referenced Aug 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I had to make the visibility for some storage item and private functions
pub(crate)for the unit test of the migration. I still feel better aboutpub(crate)thanpubbecausepuballows direct access from the runtime context.